UniqueTogheterValidator fails with partial unique indexes with ForeignKey #9779
Conversation
… partial unique-constraint against FKs
There was a problem hiding this comment.
Pull request overview
This PR adds test coverage for issue #9777, which involves UniqueTogetherValidator failing with partial unique indexes that include ForeignKey fields. The PR introduces test models and test cases to reproduce and verify the issue where Django's UniqueConstraint with conditions on ForeignKey fields should be properly validated by Django REST Framework serializers.
Key changes:
- Added new test model
UniqueConstraintWithRelationModelwith conditional unique constraints on ForeignKey fields - Added test serializer for the new model
- Added two test methods to verify constraint violation detection for both constraint scenarios
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| class Meta: | ||
| constraints = [ | ||
| models.UniqueConstraint(fields=('value', 'first_related'), condition=models.Q(second_related__isnull=True), name='unique_constraint_with_relationa'), |
There was a problem hiding this comment.
The constraint name 'unique_constraint_with_relationa' appears to be truncated or missing an underscore. Based on Django naming conventions and the pattern in line 791 ('unique_constraint_with_relationb'), this should likely be 'unique_constraint_with_relation_a' or extend to something more descriptive like 'unique_constraint_with_relation_first'.
| class Meta: | ||
| constraints = [ | ||
| models.UniqueConstraint(fields=('value', 'first_related'), condition=models.Q(second_related__isnull=True), name='unique_constraint_with_relationa'), | ||
| models.UniqueConstraint(fields=('value', 'second_related'), condition=models.Q(first_related__isnull=True), name='unique_constraint_with_relationb'), |
There was a problem hiding this comment.
The constraint name 'unique_constraint_with_relationb' appears to be truncated or missing an underscore. For consistency with Django naming conventions, this should likely be 'unique_constraint_with_relation_b' or extend to something more descriptive like 'unique_constraint_with_relation_second'.
| class TestUniqueConstraintWithRelationValidation(TestCase): | ||
| def setUp(self): | ||
| self.related_model = IntegerFieldModel.objects.create(integer=1) | ||
| self.instance_first = UniqueConstraintWithRelationModel.objects.create( | ||
| value=1, | ||
| first_related=self.related_model, | ||
| second_related=None | ||
| ) | ||
| self.instance_second = UniqueConstraintWithRelationModel.objects.create( | ||
| value=1, | ||
| first_related=None, | ||
| second_related=self.related_model | ||
| ) | ||
|
|
||
| def test_unique_constraint_with_relation_first(self): | ||
| instance = UniqueConstraintWithRelationModel(value=1, first_related=self.related_model, second_related=None) | ||
| with pytest.raises(DjangoValidationError) as excinfo: | ||
| instance.validate_constraints() | ||
|
|
||
| assert str(excinfo.value) == "{'__all__': ['Constraint “unique_constraint_with_relationa” is violated.']}" | ||
| serializer = UniqueConstraintWithRelationModelSerializer(data={ | ||
| 'value': 1, | ||
| 'first_related': self.related_model.pk, | ||
| 'second_related': None | ||
| }) | ||
| assert not serializer.is_valid() | ||
| assert serializer.errors == { | ||
| 'non_field_errors': [ | ||
| 'The fields value, first_related must make a unique set.' | ||
| ] | ||
| } | ||
|
|
||
| def test_unique_constraint_with_relation_second(self): | ||
| instance = UniqueConstraintWithRelationModel(value=1, second_related=self.related_model, first_related=None) | ||
| with pytest.raises(DjangoValidationError) as excinfo: | ||
| instance.validate_constraints() | ||
|
|
||
| assert str(excinfo.value) == "{'__all__': ['Constraint “unique_constraint_with_relationb” is violated.']}" | ||
| serializer = UniqueConstraintWithRelationModelSerializer(data={ | ||
| 'value': 1, | ||
| 'first_related': None, | ||
| 'second_related': self.related_model.pk | ||
| }) | ||
| assert not serializer.is_valid() | ||
| assert serializer.errors == { | ||
| 'non_field_errors': [ | ||
| 'The fields value, second_related must make a unique set.' | ||
| ] | ||
| } |
There was a problem hiding this comment.
The test suite is missing a positive test case to verify that the unique constraint validation passes when the constraint conditions are not violated. Consider adding a test to verify that creating a new instance with different values (e.g., value=2) or when the condition doesn't apply (e.g., both first_related and second_related are set) is allowed and passes validation.
| class TestUniqueConstraintWithRelationValidation(TestCase): | ||
| def setUp(self): | ||
| self.related_model = IntegerFieldModel.objects.create(integer=1) | ||
| self.instance_first = UniqueConstraintWithRelationModel.objects.create( | ||
| value=1, | ||
| first_related=self.related_model, | ||
| second_related=None | ||
| ) | ||
| self.instance_second = UniqueConstraintWithRelationModel.objects.create( | ||
| value=1, | ||
| first_related=None, | ||
| second_related=self.related_model | ||
| ) | ||
|
|
||
| def test_unique_constraint_with_relation_first(self): | ||
| instance = UniqueConstraintWithRelationModel(value=1, first_related=self.related_model, second_related=None) | ||
| with pytest.raises(DjangoValidationError) as excinfo: | ||
| instance.validate_constraints() | ||
|
|
||
| assert str(excinfo.value) == "{'__all__': ['Constraint “unique_constraint_with_relationa” is violated.']}" | ||
| serializer = UniqueConstraintWithRelationModelSerializer(data={ | ||
| 'value': 1, | ||
| 'first_related': self.related_model.pk, | ||
| 'second_related': None | ||
| }) | ||
| assert not serializer.is_valid() | ||
| assert serializer.errors == { | ||
| 'non_field_errors': [ | ||
| 'The fields value, first_related must make a unique set.' | ||
| ] | ||
| } | ||
|
|
||
| def test_unique_constraint_with_relation_second(self): | ||
| instance = UniqueConstraintWithRelationModel(value=1, second_related=self.related_model, first_related=None) | ||
| with pytest.raises(DjangoValidationError) as excinfo: | ||
| instance.validate_constraints() | ||
|
|
||
| assert str(excinfo.value) == "{'__all__': ['Constraint “unique_constraint_with_relationb” is violated.']}" | ||
| serializer = UniqueConstraintWithRelationModelSerializer(data={ | ||
| 'value': 1, | ||
| 'first_related': None, | ||
| 'second_related': self.related_model.pk | ||
| }) | ||
| assert not serializer.is_valid() | ||
| assert serializer.errors == { | ||
| 'non_field_errors': [ | ||
| 'The fields value, second_related must make a unique set.' | ||
| ] | ||
| } |
There was a problem hiding this comment.
The test suite is missing a test case for updating an existing instance. Following the pattern established in other validator tests (e.g., test_updated_instance_excluded_from_unique_together at line 239), there should be a test to verify that when updating an instance with the same values, it doesn't fail validation against itself.
| class TestUniqueConstraintWithRelationValidation(TestCase): | ||
| def setUp(self): | ||
| self.related_model = IntegerFieldModel.objects.create(integer=1) | ||
| self.instance_first = UniqueConstraintWithRelationModel.objects.create( | ||
| value=1, | ||
| first_related=self.related_model, | ||
| second_related=None | ||
| ) | ||
| self.instance_second = UniqueConstraintWithRelationModel.objects.create( | ||
| value=1, | ||
| first_related=None, | ||
| second_related=self.related_model | ||
| ) | ||
|
|
||
| def test_unique_constraint_with_relation_first(self): | ||
| instance = UniqueConstraintWithRelationModel(value=1, first_related=self.related_model, second_related=None) | ||
| with pytest.raises(DjangoValidationError) as excinfo: | ||
| instance.validate_constraints() | ||
|
|
||
| assert str(excinfo.value) == "{'__all__': ['Constraint “unique_constraint_with_relationa” is violated.']}" | ||
| serializer = UniqueConstraintWithRelationModelSerializer(data={ | ||
| 'value': 1, | ||
| 'first_related': self.related_model.pk, | ||
| 'second_related': None | ||
| }) | ||
| assert not serializer.is_valid() | ||
| assert serializer.errors == { | ||
| 'non_field_errors': [ | ||
| 'The fields value, first_related must make a unique set.' | ||
| ] | ||
| } | ||
|
|
||
| def test_unique_constraint_with_relation_second(self): | ||
| instance = UniqueConstraintWithRelationModel(value=1, second_related=self.related_model, first_related=None) | ||
| with pytest.raises(DjangoValidationError) as excinfo: | ||
| instance.validate_constraints() | ||
|
|
||
| assert str(excinfo.value) == "{'__all__': ['Constraint “unique_constraint_with_relationb” is violated.']}" | ||
| serializer = UniqueConstraintWithRelationModelSerializer(data={ | ||
| 'value': 1, | ||
| 'first_related': None, | ||
| 'second_related': self.related_model.pk | ||
| }) | ||
| assert not serializer.is_valid() | ||
| assert serializer.errors == { | ||
| 'non_field_errors': [ | ||
| 'The fields value, second_related must make a unique set.' | ||
| ] | ||
| } |
There was a problem hiding this comment.
The test should also verify the case where both first_related and second_related are non-null. Based on the constraint conditions (second_related__isnull=True and first_related__isnull=True), when both fields have values, neither constraint should apply and the validation should pass. This is an important edge case that demonstrates the constraint conditions are being properly evaluated.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2 participants
Description
WIP: currently just adds tests for #9777